Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

prom conversion: Support terraform outputs #49

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

Conversation

bbarnes52
Copy link
Contributor

@bbarnes52 bbarnes52 commented Jan 17, 2025

Adds a method for converting the outputs of prometheus/alertmanager conversion to terraform types that can be marshalled to HCL. Once chronoctl convert is implemented, it will output HCL if --output=hcl is set.

Copy link
Contributor Author

bbarnes52 commented Jan 17, 2025

@bbarnes52 bbarnes52 changed the title prom conversion: Support terraform outputs [WIP] prom conversion: Support terraform outputs Jan 18, 2025
@bbarnes52 bbarnes52 changed the base branch from bgb/rmrecordinggroup to 01-17-bgb_setrecordingruleslugs January 18, 2025 02:49
@bbarnes52 bbarnes52 force-pushed the bgb/test branch 4 times, most recently from 322f804 to 5747798 Compare January 21, 2025 17:38
@bbarnes52 bbarnes52 changed the title [WIP] prom conversion: Support terraform outputs prom conversion: Support terraform outputs Jan 21, 2025
@bbarnes52 bbarnes52 marked this pull request as ready for review January 21, 2025 20:20
@bbarnes52 bbarnes52 requested a review from a team as a code owner January 21, 2025 20:20
Copy link
Contributor

@prashantv prashantv left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we add tests for the TF conversion logic?

require (
github.com/alecthomas/participle/v2 v2.1.0
github.com/anyascii/go v0.3.2
github.com/chronosphereio/terraform-provider-chronosphere v1.6.2
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(no action needed) this ends up pulling in a fair few new deps. not a huge deal, but i wonder if we should move the conversion functions out of the chronosphere package into a subpackage

@@ -6,17 +6,17 @@ replace github.com/chronosphereio/chronoctl-core => ../

require (
github.com/chronosphereio/chronoctl-core v0.0.0-00010101000000-000000000000
github.com/go-openapi/analysis v0.21.4
github.com/go-openapi/analysis v0.22.0
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

updates here seem fine, just curious what they were needed for

@@ -1684,7 +1684,7 @@ func setReceiverSlugs(
// each receiver has at most one target. Returned receivers also have slugs set.
// The returned map indexes the original receivers names the new receivers for that name.
func expandReceivers(
receivers []*config.Receiver,
receivers []config.Receiver,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why this change?

Comment on lines +138 to +146
b, err := in.MarshalBinary()
if err != nil {
return nil, err
}
var tf tfmodels.Configv1Collection
err = tf.UnmarshalBinary(b)
if err != nil {
return nil, err
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we use a generic function to DRY this up?

we may be able to pass the XFromModel function as an arg to have that handled too?

return nil, err
}
out.HCLID = tfid.SafeID(tf.Slug)
out.ExecutionGroup = collectionTFID(out.ExecutionGroup)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

BucketSlug should use a collectionTFID, but ExecutionGroup is treated as a flat string, and generally isn't a reference

}
tf.HCLID = tfid.SafeID(notifier.Slug)
tfOut.EmailNotifiers = append(tfOut.EmailNotifiers, tf)
notifierTypes[notifier.Slug] = "chronosphere_email_alert_notifier"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we avoid these hardcoded values by using .Ref() and using .Type() (will probably need a typecast)
https://github.com/chronosphereio/terraform-provider-chronosphere/blob/main/chronosphere/intschema/classic_dashboard.go#L54

}
return tfid.LocalRef(tfid.Ref{
Type: "chronosphere_notification_policy",
ID: slug.Slug(),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do we need to use a TF safe ID here?

@bbarnes52 bbarnes52 force-pushed the 01-17-bgb_setrecordingruleslugs branch from 2d73136 to 652d6f3 Compare February 3, 2025 22:18
Base automatically changed from 01-17-bgb_setrecordingruleslugs to main February 3, 2025 22:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants